S3Express CreateSession Allowlist Headers #492

waahm7 · 2025-01-23T22:25:47Z

Issue #, if available:
Adding support for SSE and ReadOnly sessions for Mountpoint.

Description of changes:

As of now, the S3Express CreateSession API allows the users to set 5 headers. This PR adds an experimental support for these headers so that if the request contains these headers, we will pass them to the CreateSession call.
Updates the caching layer to include all allowed headers and we will create a new session for any difference in headers.
This API uses an allow list instead of the usual exclude list for other APIs. Mainly because we want tighter control over what we are passing to the CreateSession call and what we are caching.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2025-01-23T22:40:11Z

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.64%. Comparing base (8feda6e) to head (94fec48).

Files with missing lines	Patch %	Lines
source/s3express_credentials_provider.c	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   89.59%   89.64%   +0.04%     
==========================================
  Files          20       20              
  Lines        6287     6344      +57     
==========================================
+ Hits         5633     5687      +54     
- Misses        654      657       +3

Files with missing lines	Coverage Δ
source/s3_client.c	`91.03% <100.00%> (-0.01%)`	⬇️
source/s3_meta_request.c	`92.04% <100.00%> (-0.06%)`	⬇️
source/s3_request_messages.c	`74.36% <ø> (ø)`
source/s3express_credentials_provider.c	`92.84% <98.00%> (+0.35%)`	⬆️

... and 2 files with indirect coverage changes

dannycjones · 2025-01-24T10:07:37Z

@waahm7 we'll also want to set the S3 Express session mode, will you account for that in this PR?

waahm7 · 2025-01-24T18:24:51Z

@dannycjones Thanks, I have accounted for it in this PR.

DmitriyMusatkin · 2025-01-24T21:45:54Z

source/s3_request_messages.c

@@ -27,6 +27,7 @@ const struct aws_byte_cursor g_s3_create_multipart_upload_excluded_headers[] = {
    AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("x-amz-checksum-sha1"),
    AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("x-amz-checksum-sha256"),
    AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("if-none-match"),
+    AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("x-amz-create-session-mode"),


sse headers passed to create, upload and complete? seems kinda counter intuitive

Yes, sse headers can be the same as in CreateMPU and Upload Part. From CreateMultipart Docs:
"""
In the Zonal endpoint API calls (except CopyObject and UploadPartCopy) using the REST API, the encryption request headers must match the encryption settings that are specified in the CreateSession request. You can't override the values of the encryption settings (x-amz-server-side-encryption, x-amz-server-side-encryption-aws-kms-key-id, x-amz-server-side-encryption-context, and x-amz-server-side-encryption-bucket-key-enabled) that are specified in the CreateSession request. You don't need to explicitly specify these encryption settings values in Zonal endpoint API calls, and Amazon S3 will use the encryption settings values from the CreateSession request to protect new objects in the directory bucket.
"""

We can exclude them but it feels like needless complexity trying to differentiate between UploadPart for S3 and UploadPart for S3Express after CreateSession.

They are always excluded in the completeMPU.

DmitriyMusatkin · 2025-01-24T21:54:18Z

source/s3express_credentials_provider.c

-    if (aws_http_message_add_header(request, host_header)) {
-        goto error;
+    /* NOTE: Only for Tests.
+     * Don't add the host header for endpoint override.


why? sounds like even for overridden endpoint we still need host

The previous logic was passing the endpoint override to meta_request options, so we didn't need to add the host header. I have updated it to just add the host header from override as that makes it more understandable.

Found a bug in our host parsing logic: https://github.com/awslabs/aws-c-s3/pull/492/files#diff-5ee13dd0ee005b828bde6d60ff89582070983e3cb7e5d92389b629a9850c27f8R1033

Update the CRT libraries to the latest releases. In particular, include: * S3Express CreateSession Allowlist Headers ([awslabs/aws-c-s3#492](awslabs/aws-c-s3#492)) <details> <summary>Full CRT changelog:</summary> ``` Submodule mountpoint-s3-crt-sys/crt/aws-c-auth 5bc67797..b513db4b: > A bunch of CMake fixes (#258) > Add Account Id to Credentials (#260) > Skip Transfer-Encoding from signing (#261) Submodule mountpoint-s3-crt-sys/crt/aws-c-cal fbbe2612..7299c6ab: > Fix Findcrypto.cmake (#205) > A bunch of CMake fixes (#203) > Switch CI to use roles (#202) Submodule mountpoint-s3-crt-sys/crt/aws-c-common 7a6f5df2..0e7637fa: > A bunch of CMake fixes (#1178) > Fix heap overflow on uri parsing (#1185) > (take 2) Detect when AVX is disabled via OSXSAVE (#1184) > Fixup IPv6 validation logic (#1180) > Detect when AVX is disabled via OSXSAVE (#1182) > proof_ci.yaml must use latest upload-artifact (#1183) > change PR template to ask for clearer wording (#1177) Submodule mountpoint-s3-crt-sys/crt/aws-c-compression c6c1191e..f951ab2b: > A bunch of CMake fixes (#72) > Switch CI to use roles (#71) > chore: Modified bug issue template to add checkbox to report potential regression. (#69) Submodule mountpoint-s3-crt-sys/crt/aws-c-http fc3eded2..590c7b59: > A bunch of CMake fixes (#497) > Fix CI for GCC-13 on Ubuntu-18 (#496) > Switch CI to use roles (#494) Submodule mountpoint-s3-crt-sys/crt/aws-c-io fcb38c80..3041dabf: > A bunch of CMake fixes (#701) > Event Loop & Socket Type Multi-Support (#692) > fix typo in log message (#702) > Fix CI for GCC-13 on Ubuntu-18 (#700) > Switch CI to use roles (#698) Submodule mountpoint-s3-crt-sys/crt/aws-c-s3 a3b401bf..6eb8be53: > A bunch of CMake fixes (#480) > S3Express CreateSession Allowlist Headers (#492) > Auto - Update S3 Ruleset & Partition (#491) Submodule mountpoint-s3-crt-sys/crt/aws-c-sdkutils 1ae8664f..ba6a28fa: > A bunch of CMake fixes (#50) Submodule mountpoint-s3-crt-sys/crt/aws-checksums 3e4101b9..fb8bd0b8: > A bunch of CMake fixes (#101) > Switch CI to use roles (#100) Submodule mountpoint-s3-crt-sys/crt/aws-lc ffd6fb71..138a6ad3: > Prepare AWS-LC v1.44.0 (#2153) > Fix issue with ML-DSA key parsing (#2152) > Add support for PKCS7_set/get_detached (#2134) > Prepare Docker image for CI integration jobs (#2126) > Delete OpenVPN mainline patch from our integration build (#2149) > SHA3/SHAKE Init Updates via FIPS202 API layer (#2101) > Support keypair calculation for PQDSA PKEY (#2145) > Optimize x86/aarch64 MD5 implementation (#2137) > Check for MIPSEB in target.h (#2143) > Ed25519ph and Ed25519ctx Support (#2120) > Support for ML-DSA public key generation from private key (#2142) > Avoid mixing SSE and AVX in XTS-mode AVX512 implementation (#2140) > Remove remaining support for Trusty and Fuchsia operating systems (#2136) > ACVP test harness for ML-DSA (#2127) > Minor symbols to work with Ruby's mainline (#2132) ``` </details> ### Does this change impact existing behavior? No. ### Does this change need a changelog entry? Does it require a version change? No. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the [Developer Certificate of Origin (DCO)](https://developercertificate.org/). Signed-off-by: Alessandro Passaro <[email protected]>

waahm7 added 7 commits January 22, 2025 11:13

piping the headers

f8ffac1

More fixes

8076511

null safety

dfc4a8d

lint

b3f48c7

Add test and update mock server

7d6b98a

update readme

ff8577b

Fix tests

bea7dc4

waahm7 added 3 commits January 24, 2025 09:47

Add hash key test

30e9a18

Add support for session mode

2d74e6e

Cleanup

73c5fc2

waahm7 changed the title ~~WIP | S3Express SSE Settings for CreateSession~~ WIP | S3Express CreateSession Headers Jan 24, 2025

waahm7 added 2 commits January 24, 2025 10:07

Add experimental warning

e74da12

lint

c7975d2

waahm7 changed the title ~~WIP | S3Express CreateSession Headers~~ S3Express CreateSession Allowlist Headers Jan 24, 2025

add type hint

0b07914

waahm7 marked this pull request as ready for review January 24, 2025 18:24

waahm7 added 2 commits January 24, 2025 10:49

fix codecov action

54cecca

init and cleanup

2ccc3b2

DmitriyMusatkin reviewed Jan 24, 2025

View reviewed changes

waahm7 added 4 commits January 24, 2025 14:10

try the new host override

a9507a5

fix server

decf5f0

try setting port

2d93aef

authority instead of host

94fec48

DmitriyMusatkin approved these changes Jan 27, 2025

View reviewed changes

waahm7 merged commit aef075b into main Jan 27, 2025
35 checks passed

waahm7 deleted the express-sse-fix branch January 27, 2025 18:29

passaro mentioned this pull request Feb 5, 2025

Update CRT submodules to latest releases awslabs/mountpoint-s3#1255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3Express CreateSession Allowlist Headers #492

S3Express CreateSession Allowlist Headers #492

waahm7 commented Jan 23, 2025 •

edited

Loading

codecov-commenter commented Jan 23, 2025 •

edited

Loading

dannycjones commented Jan 24, 2025

waahm7 commented Jan 24, 2025

DmitriyMusatkin Jan 24, 2025

waahm7 Jan 24, 2025 •

edited

Loading

waahm7 Jan 24, 2025

DmitriyMusatkin Jan 24, 2025

waahm7 Jan 24, 2025

waahm7 Jan 24, 2025

S3Express CreateSession Allowlist Headers #492

S3Express CreateSession Allowlist Headers #492

Conversation

waahm7 commented Jan 23, 2025 • edited Loading

codecov-commenter commented Jan 23, 2025 • edited Loading

Codecov Report

dannycjones commented Jan 24, 2025

waahm7 commented Jan 24, 2025

DmitriyMusatkin Jan 24, 2025

Choose a reason for hiding this comment

waahm7 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

waahm7 Jan 24, 2025

Choose a reason for hiding this comment

DmitriyMusatkin Jan 24, 2025

Choose a reason for hiding this comment

waahm7 Jan 24, 2025

Choose a reason for hiding this comment

waahm7 Jan 24, 2025

Choose a reason for hiding this comment

waahm7 commented Jan 23, 2025 •

edited

Loading

codecov-commenter commented Jan 23, 2025 •

edited

Loading

waahm7 Jan 24, 2025 •

edited

Loading